net: add setTOS and getTOS to Socket #61503
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
mcollina
left a comment
mcollina
left a comment
There was a problem hiding this comment.
Good work. Can you add the documentation?
mcollina
added
the
semver-minor
|
Thanks @mcollina , is this ready to be merged? Please let me know if there is anything else you need me to adjust. |
amyssnippet
force-pushed
the
fix/61489
branch
from
42d5142 to
c0407b2
Compare
github-actions
bot
removed
the
request-ci
|
@mcollina I'm confused by the CI failures. Any ideas? Or just flaky? |
|
@ronag check now, i guess everything including ci should work. have a look. Thanks! |
ronag
added
author ready
github-actions
bot
removed
the
request-ci
|
@amyssnippet: based on your solution I assume that libuv doesn't have an api for this... maybe would be an idea to open an issue over there as well? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
i guess you are right @ronag |
|
and also i have observed some flaky ci tests here, should i report them as issues? or just paste them here so the internal team fixed them?? |
|
libuv/libuv#2536 |
|
@ronag after inspecting some of errors in ci, i rolled some changes, kindle approve the ci once again so i can futher improve the checks |
github-actions
bot
removed
the
request-ci
|
What does TOS stand for? |
|
doc/api/net.md
Outdated
| @@ -1461,6 +1461,45 @@ If `timeout` is 0, then the existing idle timeout is disabled. | |||
| The optional `callback` parameter will be added as a one-time listener for the | |||
| [`'timeout'`][] event. | |||
|
|
|||
| ### `socket.getTOS()` | |||
There was a problem hiding this comment.
Our API names in Node.js are generally more verbose than the corresponding OS APIs – would there be anything speaking against calling these socket.getTypeOfService() and socket.setTypeOfService()?
There was a problem hiding this comment.
that makes sense and i agree to that, i will update to use full names
There was a problem hiding this comment.
@addaleax i have used full names now, check the latest changes
| } else { | ||
| args.GetReturnValue().Set(uv_translate_sys_error(errno)); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
I wouldn't say this needs to block this PR, but upstreaming these additions to https://github.com/libuv/libuv might be a good follow-up change
There was a problem hiding this comment.
even i was thinking the same, if internal members of nodejs will work and do the changes over there to libuv, then additions will be fast, rather than i do.
There was a problem hiding this comment.
If you can open an issue there pointing to this PR, that will help and maybe someone picks it up.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61503 +/- ##
==========================================
- Coverage 89.82% 88.88% -0.94%
==========================================
Files 667 672 +5
Lines 203693 203928 +235
Branches 39163 39129 -34
==========================================
- Hits 182963 181259 -1704
- Misses 13061 14933 +1872
- Partials 7669 7736 +67
🚀 New features to boost your workflow:
|
7 participants
this PR implements
socket.setTOS(tos)andsocket.getTOS()innet.Socket. it needed this to support DSCP tagging (QoS) for traffic prioritization, which wasn't previously exposed in the JS API. for the implementation:tcp_wrap.ccto attemptIP_TOSfirst, and fallback toIPV6_TCLASSif that fails. This handles both IPv4 and IPv6 sockets automatically without needing a separate flag.UV_ENOSYSfor now since the headers/implementation differ there.test-net-socket-tos.js) to verify input validation and ensure the values are actually set on supported platforms.Fixes: #61489